Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix: API response for activating users returns 403 Forbidden #108

Merged
merged 3 commits into from
Apr 14, 2021
Merged

Fix: API response for activating users returns 403 Forbidden #108

merged 3 commits into from
Apr 14, 2021

Conversation

Amulya-coder
Copy link
Member

@Amulya-coder Amulya-coder commented Mar 1, 2021 •
edited
Loading

Description

Adding the decorators separately to make the activation method available without any auth credentials.

Fixes #81

Type of Change:

  • Code

Code/Quality Assurance Only

  • Bugfix (non-breaking change which fixes an issue)

Checklist:

  • My PR follows the style guidelines of this project
  • I have performed a self-review of my own code or materials

Code/Quality Assurance Only

  • My changes generate no new warnings

@Amulya-coder
Copy link
Member Author

@codesankalp can you please review it

codesankalp
codesankalp suggested changes Mar 1, 2021
View reviewed changes
Copy link
Member

@codesankalp codesankalp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QA-Checks and tests are showing as queued. Please make the suggested changes and let's see if the workflow runs again or not. @Amulya-coder

token_auth/views/register.py Outdated Show resolved Hide resolved
@codesankalp codesankalp added the Status: Changes Requested Changes are required to be done by the PR author. label Mar 1, 2021
@Amulya-coder
Copy link
Member Author

@codesankalp it's still failing!

@Amulya-coder
Copy link
Member Author

In my terminal, it's showing some errors like pylint(import-error) can this may be the reason?

@codesankalp
Copy link
Member

codesankalp commented Mar 2, 2021 •
edited
Loading

@Amulya-coder Follow the guidelines mentioned in README for QA-Checks and Testing.
And according to the workflows test, Black check and Flake8 check are failing.

@Amulya-coder
Copy link
Member Author

@codesankalp I have run black-checks and flake-checks everything looks fine. I think failing test isn't a major problem for this issue we can ignore this.

@codesankalp
Copy link
Member

@Amulya-coder This PR can't be merged if tests (QA-checks) do not pass.

@codesankalp
Copy link
Member

codesankalp commented Mar 3, 2021 •
edited
Loading

You can use the suggestions as mentioned in the actions.
Also I can't approve this if QA-checks are failing.

@Amulya-coder
Copy link
Member Author

Ok @codesankalp I will try once again.

@Amulya-coder
Copy link
Member Author

@codesankalp QA checks are passed 👍

image

@Amulya-coder
Copy link
Member Author

@codesankalp now can you please approve this.

codesankalp
codesankalp previously approved these changes Mar 3, 2021
View reviewed changes
Copy link
Member

@codesankalp codesankalp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Amulya-coder LGTM 🎉
Code Review: Done
Testing

Screenshots
image

image

image

@codesankalp codesankalp added Status: Needs Review PR needs an additional review or a maintainer's review. and removed Status: Changes Requested Changes are required to be done by the PR author. labels Mar 3, 2021
@codesankalp
Copy link
Member

@isabelcosta Needs your review in this PR.

devkapilbansal
devkapilbansal previously approved these changes Mar 16, 2021
View reviewed changes
Copy link
Member

@devkapilbansal devkapilbansal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@devkapilbansal devkapilbansal added Status: Needs Testing Work has been reviewed and needs the code tested by the quality assurance team. and removed Status: Needs Review PR needs an additional review or a maintainer's review. labels Mar 16, 2021
@codesankalp
Copy link
Member

I have tested it and it is working fine @devkapilbansal.
Can you test it also, so that I can add a ready to merge label?

@devkapilbansal
Copy link
Member

Adding Ready to Merge since this is already been tested above.

@codesankalp codesankalp added Status: Ready to Merge Work has been tested and needs a final review and merge from a repo maintainer. and removed Status: Needs Testing Work has been reviewed and needs the code tested by the quality assurance team. labels Mar 16, 2021
@codesankalp
Copy link
Member

Thanks, @Amulya-coder for your contribution. 🚀

@isabelcosta
Copy link
Member

@Amulya-coder @codesankalp @devkapilbansal this is failing the tests, why is that 🤔

@codesankalp
Copy link
Member

@isabelcosta You can trigger the workflow again but the workflow will fail due to a secret-key error.

@codesankalp
Copy link
Member

After merging of this #109
You can trigger the workflow again and it will pass.

@Amulya-coder
Copy link
Member Author

@isabelcosta locally it will pass all the build tests I have checked, here it shows due to the secret key as github requires the API key to trigger the workflow which is resolved by the codesankalp in #109

@isabelcosta
Copy link
Member

@Amulya-coder can you please update the branch? So the tests will pass?

@Amulya-coder
Copy link
Member Author

ok @isabelcosta I will update it by tomorrow.

@devkapilbansal devkapilbansal added Status: Changes Requested Changes are required to be done by the PR author. and removed Status: Ready to Merge Work has been tested and needs a final review and merge from a repo maintainer. labels Apr 2, 2021
@Amulya-coder
Copy link
Member Author

@isabelcosta I have updated the branch, can you please review it, sorry for the delay.

@codesankalp codesankalp added Status: Needs Review PR needs an additional review or a maintainer's review. and removed Status: Changes Requested Changes are required to be done by the PR author. labels Apr 14, 2021
isabelcosta
isabelcosta approved these changes Apr 14, 2021
View reviewed changes
Copy link
Member

@isabelcosta isabelcosta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! Thank you for your contribution @Amulya-coder 🎉

@isabelcosta isabelcosta added Status: Ready to Merge Work has been tested and needs a final review and merge from a repo maintainer. and removed Status: Needs Review PR needs an additional review or a maintainer's review. labels Apr 14, 2021
@isabelcosta
Copy link
Member

I will merge because latest changes were only to style with black formatting, and this was tested already by @codesankalp 🚀

@isabelcosta isabelcosta merged commit 2530865 into anitab-org:develop Apr 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@isabelcosta isabelcosta isabelcosta approved these changes

@codesankalp codesankalp codesankalp approved these changes

@devkapilbansal devkapilbansal Awaiting requested review from devkapilbansal

Assignees
No one assigned
Labels
Status: Ready to Merge Work has been tested and needs a final review and merge from a repo maintainer.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bug: API response for activating users returns 403 Forbidden
4 participants
@Amulya-coder @codesankalp @devkapilbansal @isabelcosta